Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for broken model.Model#remove by foreign key #379

Closed
wants to merge 1 commit into from

Conversation

morokhovets
Copy link

Try something like:

geddy.model.Post.remove({userId: this.id})

It will break with

/usr/local/lib/node_modules/geddy/node_modules/model/lib/query/query.js:184
          datatype = descr.datatype;
                          ^
TypeError: Cannot read property 'datatype' of undefined

Tracking this down I've found that in geddy/lib/app/index.js there's a hook on 'beforeRemove' event that changes query conditions object adding 'model' and 'event' attributes in there.
The patch is just a simple workaround for this problem.
Not sure of the best way to fix.

@larzconwell
Copy link
Contributor

Actually in the docs .remove doesn't take queries, only an ID so maybe that has something to do with it?

@morokhovets
Copy link
Author

Well, you're probably right. I didn't even consider this.
It's not clear from documentation how to get a db connection to execute raw SQL. And I couldn't get it reading sources.
The only solution left is to async.each(idsCollection, Model.remove)?

It turns out almost every feature of 'model' I'm trying to use is broken or incomplete.
For example, try to use eager loading for fields with non-default names. It will break.
Eager loading for more than one level of associations is not supported.
Define 'bindings': {model: 'UserBinding'} and you will get 'getbindings' and 'addbinding' methods in model (note casing in names).
I understand that I should report more issues for this. But good report takes time that I don't have after hunting for so many bugs.
Guys, you SHOULD use unit testing. It really helps.

@ben-ng
Copy link

ben-ng commented Jun 4, 2013

You can remove multiple items in a single call, pass in an array of IDs. It's in the tests.

On Jun 4, 2013, at 8:39 AM, Alexander Morokhovets notifications@github.com wrote:

Well, you're probably right. I didn't even consider this.
It's not clear from documentation how to get a db connection to execute raw SQL. And I couldn't get it reading sources.
The only solution left is to async.each(idsCollection, Model.remove)?

It turns out almost every feature of 'model' I'm trying to use is broken or incomplete.
For example, try to use eager loading for fields with non-default names. It will break.
Eager loading for more than one level of associations is not supported.
Define 'bindings': {model: 'UserBinding'} and you will get 'getbindings' and 'addbinding' methods in model (notice casing in names).
I understand that I should report more issues for this. But good report takes time that I don't have after hunting for so many bugs.
Guys, you SHOULD use unit testing. It really helps.


Reply to this email directly or view it on GitHub.

@morokhovets
Copy link
Author

ben-ng, It would be two calls actually. Get IDs + delete.

@morokhovets
Copy link
Author

ben-ng,
your solution is not working in geddy/model + postgresql environment.
Callback after #remove gets this error:

{ [error: syntax error at or near ")"]
  length: 83,
  name: 'error',
  severity: 'ERROR',
  code: '42601',
  detail: undefined,
  hint: undefined,
  position: '67',
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'scan.l',
  line: '1001',
  routine: 'scanner_yyerror' }

I've tried both: Model.remove(idsArray) and Model.remove({id: idsArray})

@mde
Copy link
Contributor

mde commented Jun 29, 2013

Sorry it's taken so long to have a look at this. There seem to be a lot of separate issues here. It looks like from your patch that the breakage happens only in the realtime code. Is that correct?

We do have both unit and integration tests, although we don't have as much coverage as we'd like. There's a shared integration test for removing by an ID, or an array of IDs, and they pass in Postgres.

As far as the other issues:

-- What does "eager loading for fields with non-default names" mean? Are you talking about named associations?
-- Eager loading multiple levels of association -- I've opened an issue: geddy/model#66
-- I don't understand what you mean by "define 'bindings'" -- are you talking about adding an association for a model definition?

@morokhovets
Copy link
Author

It looks like from your patch that the breakage happens only in the realtime code. Is that correct?

I'm not sure of what you mean. If "realtime code" == "code executed on client" then no it's a server side controller action code.

There's a shared integration test for removing by an ID, or an array of IDs, and they pass in Postgres.

Well when I was elaborating on this bug It was clear from the source code that it would break.
I've ended up using async.each(ids, geddy.model.myModel.remove). Did not try reproduce it since.

What does "eager loading for fields with non-default names" mean? Are you talking about named associations?

It turned out that bug occurs when you try to use associations with compund class names, e.g:

Server = ->
  hasMany "DbConfigs"

geddy.model.Server.first {includes: "dbConfigs"}, (err, server) =>
   console.log server.dbConfigs #=> undefined

geddy.model.Server.first {includes: "db_configs"}, (err, server) => # snake case
   console.log server.db_configs #=> actual array of configs, snake case only as well

I don't understand what you mean by "define 'bindings'"

User = ->
  hasMany 'bindings', model: 'UserBinding'

geddy.model.User.first {}, (err, user) =>
  user.addBinding geddy.model.UserBinding.create(blah: "blah") #=> addBinding is not a function

geddy.model.User.first {}, (err, user) =>
  user.addbinding geddy.model.UserBinding.create(blah: "blah") #=> OK

Thanks for your response!

@mde
Copy link
Contributor

mde commented Jun 29, 2013

All right, so I've pushed a Geddy update that fixes that problem of reference pollution in app/index.js. This is on NPM, v0.8.12. Let me know if this fixes your problem removing associated items by their foreign key.

For future reference, the remove method in the Postgres adapter simply uses the same query transformation as it does for find and update, so you can remove by id, array or ids, or any query criteria you want: https://github.com/mde/model/blob/master/lib/adapters/sql/postgres.js#L266

The problem with the 'includes' query condition has an (already fixed) issue here: geddy/model#68

I'm going to have to dig a little more on the issue you're having with 'addBinding' -- we have tests that cover this (using 'addProfile'), and they work correctly.

@mde
Copy link
Contributor

mde commented Jul 1, 2013

Your problem with your 'Binding' association is that you lowercased the name in the hasMany call. The current code expected a model definition name (i.e., the constructor). I've updated the code to normalize this better. Fix is in 15c506dcf2ff200c6edeae082213247929a6a765.

@mde mde closed this Jul 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants